fix: mandatory provider_skill_id and strict catalog validation#317
fix: mandatory provider_skill_id and strict catalog validation#317yacosta738 merged 4 commits intomainfrom
Conversation
- Update spec.md: make provider_skill_id mandatory in JSON. - Fix skill.rs: use full ID for local source resolution. - Fix catalog.rs: strictly validate provider_skill_id. - Update tests to align with new rules. Co-authored-by: yacosta738 <33158051+yacosta738@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughSummary by CodeRabbit
WalkthroughRequire Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/skills/catalog.rs`:
- Around line 790-794: validate_provider_skill_id currently only checks for at
least one '/', allowing traversal or malformed values like '../x/y', '/x/y', or
'x/../y'; update validate_provider_skill_id to split provider_skill_id on '/'
and enforce at least two segments, ensure no segment is empty, no segment equals
"." or "..", and reject any input that starts with or ends with a slash;
additionally restrict each segment to a safe character class (e.g., alphanumeric
plus dot, underscore, hyphen) so only path-safe relative segments are allowed
(apply these checks inside validate_provider_skill_id and return an error/bail
for any violation).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed3a0f34-d1cb-4b25-9001-0449caaa1a0a
📒 Files selected for processing (6)
openspec/specs/skill-recommendations/spec.mdspec.patchsrc/commands/skill.rssrc/skills/catalog.rstests/integration/skill_suggest.rstests/unit/suggest_catalog.rs
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openspec/specs/skill-recommendations/spec.md (1)
854-875:⚠️ Potential issue | 🟠 MajorThe compatibility wording now contradicts the requirement.
This section says
provider_skill_idMUST be serialized, but the scenario below says the field is “allowed but not required.” That makes the contract ambiguous, and the earlier JSON example still omits the field entirely. Reword this as “producers MUST emitprovider_skill_id; existing consumers MAY ignore it” and update the example to match.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/specs/skill-recommendations/spec.md` around lines 854 - 875, Update the spec text to remove the contradiction by making producers required to emit provider_skill_id and consumers optional to read it: change the sentence to "producers MUST emit `provider_skill_id`; existing consumers MAY ignore it" and update the JSON example and the Scenario descriptions for `SuggestJsonRecommendation` so the example includes a `"provider_skill_id": "dallay/agents-skills/makefile"` alongside `"skill_id"`, `"matched_technologies"`, `"reasons"`, and `"installed"`, keeping those fields unchanged; ensure references to `SuggestJsonRecommendation`, `provider_skill_id`, `skill_id`, `matched_technologies`, `reasons`, and `installed` are updated so the contract is unambiguous about emission vs. consumer handling.
♻️ Duplicate comments (1)
src/skills/catalog.rs (1)
1039-1044:⚠️ Potential issue | 🟠 Major
provider_skill_idvalidation is still too weak.Counting slashes alone still accepts malformed IDs like
../x/y,/x/y, orowner//skill. That misses the “strict validation” goal here and still leaves the new full-ID lookup path exposed to traversal-style values. Validate path components instead of separator count: require a relative path, reject empty/./..segments, and reject leading/trailing slashes or backslashes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/skills/catalog.rs` around lines 1039 - 1044, The current validate_provider_skill_id function (validate_provider_skill_id(provider_skill_id: &str)) is too weak; change it to validate components instead of counting slashes: ensure the string does not start or end with '/' or '\\', split on '/' and require at least two components, and for each component reject empty strings and the single-segment values "." or ".."; if any check fails, bail with a clear error message. Keep the function signature and Result return type, and perform these checks before returning Ok(()) so traversal-styled or malformed IDs like "/x/y", "../x/y", "owner//skill", or segments with "."/".." are rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/commands/skill.rs`:
- Around line 945-947: The fallback that uses AGENTSYNC_TEST_SKILL_SOURCE_DIR
and PathBuf::from(source_root).join(id) doesn't cover embedded catalog installs
because the catalog branch (catalog.get_skill_definition(id)) still resolves
test sources via resolve_catalog_install_source(..., local_skill_id, ...);
update resolve_catalog_install_source (and any code in the
catalog.get_skill_definition path) to also try the same
AGENTSYNC_TEST_SKILL_SOURCE_DIR layouts (both source_root.join(id) and the
alternative embedded layout used by embedded skills like "rust-async-patterns"),
or refactor the lookup into a shared helper that probes both layouts and replace
the current per-branch logic so embedded-catalog installs are discovered
consistently.
---
Outside diff comments:
In `@openspec/specs/skill-recommendations/spec.md`:
- Around line 854-875: Update the spec text to remove the contradiction by
making producers required to emit provider_skill_id and consumers optional to
read it: change the sentence to "producers MUST emit `provider_skill_id`;
existing consumers MAY ignore it" and update the JSON example and the Scenario
descriptions for `SuggestJsonRecommendation` so the example includes a
`"provider_skill_id": "dallay/agents-skills/makefile"` alongside `"skill_id"`,
`"matched_technologies"`, `"reasons"`, and `"installed"`, keeping those fields
unchanged; ensure references to `SuggestJsonRecommendation`,
`provider_skill_id`, `skill_id`, `matched_technologies`, `reasons`, and
`installed` are updated so the contract is unambiguous about emission vs.
consumer handling.
---
Duplicate comments:
In `@src/skills/catalog.rs`:
- Around line 1039-1044: The current validate_provider_skill_id function
(validate_provider_skill_id(provider_skill_id: &str)) is too weak; change it to
validate components instead of counting slashes: ensure the string does not
start or end with '/' or '\\', split on '/' and require at least two components,
and for each component reject empty strings and the single-segment values "." or
".."; if any check fails, bail with a clear error message. Keep the function
signature and Result return type, and perform these checks before returning
Ok(()) so traversal-styled or malformed IDs like "/x/y", "../x/y",
"owner//skill", or segments with "."/".." are rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 61494ac4-d999-4cab-b44f-5d5a4313ea74
📒 Files selected for processing (5)
openspec/specs/skill-recommendations/spec.mdsrc/commands/skill.rssrc/skills/catalog.rstests/integration/skill_suggest.rstests/unit/suggest_catalog.rs
- Fix rustfmt in suggest_catalog.rs (line formatting) - Fix local source resolution to use full provider_skill_id path in AGENTSYNC_TEST_SKILL_SOURCE_DIR - Strengthen provider_skill_id validation to reject malformed IDs (empty segments, ., .., leading/trailing slashes) - Update spec to clarify producer MUST emit provider_skill_id, consumers MAY ignore it - Update JSON example in spec to include provider_skill_id
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/unit/suggest_catalog.rs (1)
1000-1011:⚠️ Potential issue | 🟡 MinorTest fixture uses invalid
provider_skill_idformat that will be silently rejected.
provider_skill("makefile", "makefile")creates a skill withprovider_skill_id = "makefile", which violates the new validation requiring at least two/-separated segments. Sinceoverlay_cataloguses lenient validation, this entry will be silently skipped.The test may pass coincidentally (rust detection falls back to embedded catalog), but the provider overlay isn't being applied as intended.
🔧 Proposed fix
fn recommendation_catalog(&self) -> Result<Option<ProviderCatalogMetadata>> { let detect = DetectionRules { config_files: Some(vec!["Makefile".to_string()]), ..Default::default() }; Ok(Some(ProviderCatalogMetadata { provider: "detect-metadata-provider".to_string(), version: "2026.03".to_string(), schema_version: "v1".to_string(), - skills: vec![provider_skill("makefile", "makefile")], + skills: vec![provider_skill("dallay/agents-skills/makefile", "makefile")], technologies: vec![ProviderCatalogTechnology { id: "make".to_string(), name: "Make".to_string(), - skills: vec!["makefile".to_string()], + skills: vec!["dallay/agents-skills/makefile".to_string()], detect: Some(detect), min_confidence: Some("low".to_string()), reason_template: None, }], combos: vec![], })) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/suggest_catalog.rs` around lines 1000 - 1011, The test fixture is creating a skill with provider_skill_id "makefile" which fails the new validation requiring at least two '/'-separated segments; update the call to provider_skill in tests/unit/suggest_catalog.rs (the provider_skill(...) invocation that constructs the skills vec) to use a valid provider_skill_id with at least two segments (e.g. "provider/makefile" or "provider/make/skill") so the overlay_catalog will accept and apply the provider overlay as intended; keep the rest of the skill arguments the same.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/unit/suggest_catalog.rs`:
- Around line 1000-1011: The test fixture is creating a skill with
provider_skill_id "makefile" which fails the new validation requiring at least
two '/'-separated segments; update the call to provider_skill in
tests/unit/suggest_catalog.rs (the provider_skill(...) invocation that
constructs the skills vec) to use a valid provider_skill_id with at least two
segments (e.g. "provider/makefile" or "provider/make/skill") so the
overlay_catalog will accept and apply the provider overlay as intended; keep the
rest of the skill arguments the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4158ebaf-7a66-4e64-ba1e-0457d72d7e3e
📒 Files selected for processing (4)
openspec/specs/skill-recommendations/spec.mdsrc/skills/catalog.rssrc/skills/provider.rstests/unit/suggest_catalog.rs
1 participant
Implemented mandatory provider_skill_id in JSON output, updated specification for backward compatibility requirements, fixed local source resolution to use full provider IDs, and added strict validation for provider_skill_id format in the catalog. All tests passed.
PR created automatically by Jules for task 959229972059906167 started by @yacosta738